Develop#152
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! 😊 There are a few critical issues that need to be addressed before your code can be approved. Most importantly, the logic for storing expenses in db/expense.json currently overwrites the file with each new submission instead of maintaining a list of all expenses. This means only the latest expense is saved, which does not meet the core requirements of the task. Additionally, there is a minor but important mistake in setting the response header for JSON error responses—you should use res.setHeader('Content-Type', 'application/json'); instead of res.hasHeader(...). Please update your code to always store expenses as an array (even if there is only one), and ensure the response headers are set correctly. Once these issues are fixed, your solution will be much stronger! Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| if (Object.keys(parsedData).length) { | ||
| parsedData = [parsedData]; |
There was a problem hiding this comment.
There is a logic issue here: if parsedData is an object and has keys, you convert it to an array with just that object, but then immediately push dataObject, resulting in an array with the original object and the new data. However, this logic will never be reached if parsedData is already an array (see line 90). Consider simplifying this logic to always use an array for storing expenses, as mixing objects and arrays can cause confusion and errors.
| } else if (!Object.keys(parsedData).length) { | ||
| parsedData = dataObject; |
There was a problem hiding this comment.
Here, if parsedData is an object with no keys, you set parsedData = dataObject, which means you will overwrite the data with a single object instead of an array. This is inconsistent with the intended structure for storing multiple expenses. You should always store expenses as an array, even if there is only one.
| }); | ||
| } catch (err) { | ||
| res.statusCode = 400; | ||
| res.hasHeader('Content-Type', 'application/json'); |
There was a problem hiding this comment.
There is a mistake here: res.hasHeader('Content-Type', 'application/json'); does not set the header. You should use res.setHeader('Content-Type', 'application/json'); to properly set the response header for JSON error responses.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! There is still a critical issue: after adding the new expense to the array, the code overwrites the array with just the latest expense object (parsedData = dataObject;). This means only the most recent expense is saved, and previous ones are lost. Please remove that line and make sure you always store and update an array of expenses in db/expense.json. Once you fix this, your solution will fully meet the requirements. Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| if (typeof parsedData === 'object') { | ||
| if (Array.isArray(parsedData)) { | ||
| parsedData.push(dataObject); | ||
| } | ||
|
|
||
| parsedData = dataObject; | ||
| } |
There was a problem hiding this comment.
Critical issue: After checking if parsedData is an array and pushing the new expense, the code immediately overwrites parsedData with dataObject. This discards the array and only saves the latest expense. You should remove the line parsedData = dataObject; and ensure that expenses are always stored as an array, appending new expenses to it.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your submission! 🎉 Your code meets most of the requirements, and you have implemented the core functionality using only Node.js core modules, serving the HTML form and handling requests correctly. However, there is a critical issue that you need to address: currently, your code overwrites the entire expense.json file with only the latest expense, causing all previous expenses to be lost. According to the task description, you must persist all submitted expenses by appending each new expense to the existing array in db/expense.json, not overwriting it.
Please review the comments and make sure to fix this data persistence logic in your next iteration. This is essential for the correct functionality of your application. All other requirements are met, and your code structure is clear and easy to follow.
Motivation:
We're approving this PR to help you move forward with your learning journey. Please take the time to address the critical issue mentioned above in your future work to avoid similar problems. Keep up the good work, and don't hesitate to consult the Node.js documentation on file system operations for more guidance. You're making great progress—just remember to always check that your data persistence logic matches the requirements! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (typeof parsedData === 'object') { | ||
| parsedData = dataObject; |
There was a problem hiding this comment.
Critical issue: Here, you overwrite the entire parsedData array with just the new dataObject. This means only the latest expense is saved and all previous ones are lost. Instead, you should push dataObject to parsedData (which should be an array). This is required by the task description and checklist.
2 participants
No description provided.